-
Notifications
You must be signed in to change notification settings - Fork 4.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
NavigationMenu: <LinkControl /> integration. #18062
Conversation
934bda5
to
5e85b47
Compare
I notice this is a draft, so there's probably more to do, but I've given this a quick test to help identify those things. A few comments:
|
37b5e9c
to
c7b5364
Compare
c7b5364
to
f3cc035
Compare
@retrofox I've pushed a commit that brings this a bit closer. A summary of the changes:
It mostly seems to work 👍. The main issue I'm seeing at the moment is an error when trying to select one of the suggestions using the keyboard (mouse works ok). I'm unsure if this is a problem only in this branch or if it needs to be fixed upstream in #17846. I think if that can be resolved and all the new components + props on existing components in #17846 marked as experimental it's pretty close (cc @getdave). |
Thanks, @talldan 👍 |
Let me keep the searcher there just for dev purposes. |
1ce56b5
to
b438028
Compare
f31293f
to
a88b994
Compare
I'd like to know your opinions about how to continue this implementation, which in short takes over of adding/editing a link in the Menu Item. Points to discuss, relevant changes, suggestions, questions:
I've updated the gif in the PR description, just in case. Thanks in advance. |
c858e3b
to
275a43c
Compare
This will need a careful rebase now that #17846 is merged to master. Not sure what the best approach would be. |
@retrofox Looks like there are a number of Design/UX related questions that need discussions and answers.
I discussed having the ability to remove/clear a link in the
Again, another real-world scenario reveals potential changes required to the
This mirrors the existing implementations of a "link interface" within Gutenberg. The idea is that you don't click the "selected" link and get taken away from the Block Editor interface. I'd say it's correct and should remain "as is" unless there is a problem with it.
Doesn't seem like that could cause any harm and it could be useful in the future. |
Addresses #18062 (comment)
open link control and show fake placeholder when it's a new iotem
It adds a hacky solution to deal with both events that happen at the same time: LinkControl.onClose and ToolbatButton.onClick, removing the useState ussage in favor adding a setTimeout call.
Co-Authored-By: Daniel Richards <[email protected]>
Co-Authored-By: Daniel Richards <[email protected]>
`fetchSearchSuggestions` seems to have been accidentally re-added. It’s not required. Addresses #18062 (comment)
c0b82b5
to
c34be01
Compare
Not sure why github added me as co-author to every commit just for rebasing 🤷♂ |
Great to see this in! |
Great work on this @retrofox! 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 Thanks for all the help @talldan and @draganescu! |
Description
< LinkControl />
and<NavigationItemMenu />
integration.This branch contains changes of both respective branches:
#17986#17846Screenshots
Types of changes
TODOs
URLInput
byLinkControl
The following issues will be handled in a separated PR:
Checklist: